feat(tx_cache): surface auth token retrieval errors#116
feat(tx_cache): surface auth token retrieval errors#116init4samwise wants to merge 3 commits intomainfrom
Conversation
ENG-1798: Add BuilderTxCacheError enum to properly surface auth token retrieval failures instead of silently using an empty string. Changes: - Add BuilderTxCacheError with TokenRetrieval variant wrapping RecvError - Add TxCache variant wrapping existing TxCacheError - Replace unwrap_or_else with ? operator to propagate errors - Update local Result type alias to use BuilderTxCacheError This is a breaking change for consumers that expect the old Result type, but surfaces the actual error cause for better debugging.
Evalir
requested review from
Evalir and
prestwich
and removed request for
prestwich
Fraser999
left a comment
Fraser999
left a comment
There was a problem hiding this comment.
The BuilderTxCacheError::TxCache(TxCacheError::Reqwest(err)) issue is a blocker I think.
|
|
||
| impl From<reqwest::Error> for BuilderTxCacheError { | ||
| fn from(err: reqwest::Error) -> Self { | ||
| BuilderTxCacheError::TxCache(TxCacheError::Reqwest(err)) |
There was a problem hiding this comment.
By not using from, we unconditionally make this a TxCacheError::Reqwest error. Using from means we'll set it to whichever of TxCacheError::NotFound, TxCacheError::NotOurSlot or TxCacheError::Reqwest is appropriate,
| BuilderTxCacheError::TxCache(TxCacheError::Reqwest(err)) | |
| BuilderTxCacheError::TxCache(TxCacheError::from(err)) |
| use tracing::instrument; | ||
|
|
||
| /// Result type for [`BuilderTxCache`] operations. | ||
| pub type Result<T> = std::result::Result<T, BuilderTxCacheError>; |
There was a problem hiding this comment.
| pub type Result<T> = std::result::Result<T, BuilderTxCacheError>; | |
| pub type Result<T> = core::result::Result<T, BuilderTxCacheError>; |
| assert!(matches!( | ||
| bundles, | ||
| BuilderTxCacheError::TxCache(TxCacheError::NotOurSlot) | ||
| )); |
There was a problem hiding this comment.
I don't think this matches the new behaviour, but the test is ignored, so not picked up in CI.
| #[derive(Debug, Error)] | ||
| pub enum BuilderTxCacheError { | ||
| /// Failed to retrieve the authentication token. | ||
| #[error("failed to retrieve auth token: {0}")] |
There was a problem hiding this comment.
this isn't a "failed to retrieve" right? this error is permanent and persistent, and implies something has gone wrong with the background auth task resulting in the sender being dropped?
4 participants
Summary
Surfaces auth token retrieval errors instead of silently using an empty string.
Changes
BuilderTxCacheErrorenum with:TokenRetrievalvariant wrappingwatch::error::RecvErrorTxCachevariant wrapping existingTxCacheErrorunwrap_or_elsewith?operator to propagate errorsResulttype alias to useBuilderTxCacheErrorBreaking Change
This changes the return type from
signet_tx_cache::error::Resultto a new localResulttype. Consumers will need to handle the newBuilderTxCacheErrortype.Why
Per the ticket: "Right now, an empty string is used if the token retrieval fails, placing the responsibility of returning an appropriate error on the service being called. We should wrap the original error type, and add a variant that points out that there was a problem retrieving the token instead, surfacing the inner Recv error."
Closes ENG-1798